Optimize delta binary decoder in the case where bitwidth=0#9477
Draft
etseidl wants to merge 10 commits intoapache:mainfrom
Draft
Optimize delta binary decoder in the case where bitwidth=0#9477etseidl wants to merge 10 commits intoapache:mainfrom
etseidl wants to merge 10 commits intoapache:mainfrom
Conversation
Contributor
Author
|
Not seeing the huge improvement from arrow-cpp, but still a nice speedup, and it doesn't seem to be impacting cases where the optimization can't be used. New benchmarks on my workstation (x86 i7-12700K) comparing main (no_opt) to this branch (opt) And the rest of the binary packed benches Details |
Contributor
Author
|
Currently unknown what impact this optimization will have on the other delta encodings. There could be a good speedup for situations like constant length strings + DELTA_LENGTH_BYTE_ARRAY (think UUIDs or hashes), as well as long runs of the same prefix or long runs of strings with no shared prefix with DELTA_BYTE_ARRAY. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR optimizes DELTA_BINARY_PACKED decoding for the common case where a miniblock has bit_width == 0, aiming to match speedups observed in Arrow C++ and address issue #9476.
Changes:
- Add a fast-path in the delta binary packed decoder for
bit_width == 0miniblocks (including constant-value and arithmetic-progression cases). - Add larger regression tests covering constant, increasing, and mixed patterns for
Int32/Int64. - Add new benchmark cases targeting constant and monotonically increasing delta-encoded pages.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
parquet/src/encodings/decoding.rs |
Adds bit_width == 0 decoding fast-path and new large tests for delta decoding correctness. |
parquet/benches/arrow_reader.rs |
Adds a page generator and benchmark cases for delta binary packed constant/increasing patterns. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Dandandan
reviewed
Feb 28, 2026
parquet/src/encodings/decoding.rs
Outdated
| if bit_width == 0 { | ||
| let min_delta = self.min_delta.as_i64()?; | ||
| if min_delta == 0 { | ||
| for v in &mut buffer[read..read + batch_read] { |
Contributor
There was a problem hiding this comment.
this can use .fill() (if not faster, at least looks nice)
Contributor
Author
There was a problem hiding this comment.
I had done that, but on my laptop it was actually slower for some reason...I'll check again on a more modern processor. I do agree it would look nicer.
Contributor
Author
There was a problem hiding this comment.
Ok, on my workstation fill is a bit faster. On my laptop it's a bit slower. I'll go with the workstation results 😁
Dandandan
reviewed
Feb 28, 2026
| // Kudos to @pitrou for the idea https://github.com/apache/arrow/pull/49296 | ||
| if bit_width == 0 { | ||
| let min_delta = self.min_delta.as_i64()?; | ||
| if min_delta == 0 { |
Contributor
There was a problem hiding this comment.
Can we repeat the min_delta==0 optimization below as well for bit_width > 0 ?
Contributor
Author
There was a problem hiding this comment.
I can try. The theory, however, is that the speed up is from not having to use the previous value to calculate the current. I'll see if there's any speedup from avoiding the additional wrapping_add.
Contributor
|
I think the code looks good! Lefr 2 comments |
Contributor
|
run benchmark arrow_reader |
|
🤖 |
|
🤖: Benchmark completed Details
|
Dandandan
reviewed
Mar 2, 2026
| assert_eq!(count, EXPECTED_VALUE_COUNT); | ||
| }); | ||
|
|
||
| // binary packed same value |
Contributor
There was a problem hiding this comment.
Can you submit a PR for just these benchmarks, so we can run the benchmarks on this PR?
Contributor
Author
There was a problem hiding this comment.
I do have a separate branch for the benches (and will submit separately), but combined them in this draft. I'd like to do some tests with the other delta encodings to see if the optimization here has an impact.
etseidl
force-pushed
the
delta_binary_bit_zero
branch
from
7faa0d5 to
6fc5504
Compare
etseidl
force-pushed
the
delta_binary_bit_zero
branch
from
6fc5504 to
70b9772
Compare
etseidl
force-pushed
the
delta_binary_bit_zero
branch
from
7e08a52 to
65aef7b
Compare
etseidl
force-pushed
the
delta_binary_bit_zero
branch
from
65aef7b to
3b016c4
Compare
etseidl
force-pushed
the
delta_binary_bit_zero
branch
from
3b016c4 to
37cfe15
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
Explore if we can achieve the speedups seen in arrow-cpp (apache/arrow#49296).
What changes are included in this PR?
Adds special cases to the delta binary packed decoder when bitwidth for a miniblock is 0. The optimization avoids relying on previous values to decode current ones.
Are these changes tested?
Yes, tests have been added, as well as new benchmarks.
Are there any user-facing changes?
No